ETT-1394 address warning: "CGI::Param called in list context"#234
ETT-1394 address warning: "CGI::Param called in list context"#234moseshll wants to merge 7 commits into
Conversation
- Include `mb/t/` in `perl-test`
- Fix and tests for `handle_ANALYTICS_REPORT_URL_PI`
…LYTICS_REPORT_URL_PI`
- Not really testable for reasons cited in ticket
- No tests because the method is enormous and the prospect of scaffolding a unit test is daunting.
There was a problem hiding this comment.
The actual fixes look good.
Looking at the tests, it doesn't seem like they'd fail if the changes in the code weren't there. Maybe worth considering if there's any reasonable way to test that there are no warnings, if it's easy (see e.g. https://perlmaven.com/test-for-warnings-in-a-perl-module)? That said, the tests can at least serve as regression tests if nothing else, if we aren't easily able to test the lack of warnings.
| foreach my $param ( $cgi->param ) { | ||
| if ( $param =~ m,q[0-9]|field[0-9]|anyall[0-9]|op[0-9]|lmt, ) { | ||
| $tempCgi->param($param, $cgi->param($param)); | ||
| $tempCgi->param($param, $cgi->multi_param($param)); |
There was a problem hiding this comment.
I could just call scalar here. I don't know if it is possible to ever have multiple values for the params referenced above, and additionally I don't know if it matters for the analytics report URL, or indeed if we even care.
…ere are none in the PIFiller tests. - We can probably use this approach when we get around to the Date::Manip warnings, and anything else clogging the logs.
|
@aelkiss This latest addition was a useful one, as noted, when we tackle Date::Manip and other nonsense clogging up the logs. |
| }; | ||
|
|
||
| subtest 'check for accumulated warnings' => sub { | ||
| is(scalar @warnings, 0, 'no warnings encountered'); |
There was a problem hiding this comment.
The approach of checking for warnings looks good; I would feel better about putting the check in each subtest rather than its own thing though, since typically we want tests to be isolated/not dependent on other tests (& likewise in ListUtils.t)
There was a problem hiding this comment.
Addressed in the latest commit.
- Clean up and fix some outdated comments.
2 participants
Include
mb/t/inperl-testAddress and test
LS::PIFiller::ListSearchResults::handle_ANALYTICS_REPORT_URL_PIAddress and test
MBooks::PIFiller::ListUtils::handle_ANALYTICS_REPORT_URL_PIAddress issue in
MBooks::Operation::LogoutTrap::redirect_and_exitexitand it appears its call toMBooks::View::P_redirect_HTTPalso callsexit.Test::TrapandTest::Exitbut to no avail.Address issue in
PT::Prolog::RunThere are no other issues flagged in the error logs I could find in the past 30 days.